Skip to content

fix(core): prevent cross-origin Web Audio capture from silencing audio - #3481

Merged
miguel-heygen merged 4 commits into
mainfrom
fix/web-audio-cross-origin-silence-v2
Aug 26, 2026
Merged

fix(core): prevent cross-origin Web Audio capture from silencing audio#3481
miguel-heygen merged 4 commits into
mainfrom
fix/web-audio-cross-origin-silence-v2

Conversation

@miga-heygen

Copy link
Copy Markdown
Contributor

Summary

  • Detect cross-origin <audio> elements that would be silently muted by Web Audio's CORS policy
  • Route them to decode-only or native fallback automatically — no author opt-in needed
  • Surface bypass diagnostics via hyperframes check

Takes over #3459 with the data-native-audio escape hatch removed per review feedback — automatic detection covers the use cases.

Closes #3458

Original-Author: desenmeng
Co-Authored-By: desenmeng desenmeng@users.noreply.github.com
Co-Authored-By: Miga noreply@anthropic.com

Classify each <audio> element before Web Audio capture: same-origin,
CORS-opted-in, or a non-http(s) scheme stays on the primary
createMediaElementSource() path; cross-origin media without a
crossorigin opt-in withholds that call (the Web Audio spec makes such a
node output silence without throwing) and falls back to fetch +
decodeAudioData, preserving the FX graph whenever the server allows
CORS. Recheck the route at the transport's irreversible capture
boundary, and account for currentSrc, src, and <source> candidates the
same way the HTML resource-selection algorithm does.

Emit a stable preview diagnostic (`runtime_web_audio_bypass`) at media
discovery time, not only from playback scheduling, so `hyperframes
check` — which seeks but never plays — can surface it as a
`web_audio_bypass` finding. Diagnostics are suppressed during export
rendering, where the producer mixes audio offline and already applies
the FX chain. The existing non-unit-rate fail-closed rule stays scoped
to fx-chain/automation so this fix does not newly mute grouped or
above-unity tracks.

Takes over #3459 with the data-native-audio escape hatch removed per
review feedback: the automatic cross-origin detection already covers
the cases that mattered, so the extra per-element opt-in attribute,
its route-classifier branch, and its diagnostic path are dropped in
favor of a single automatic behavior.

Fixes #3458

Original-Author: desenmeng
Co-Authored-By: desenmeng <desenmeng@users.noreply.github.com>
Co-Authored-By: Miga <noreply@anthropic.com>

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES

Reasoning: The guard only protects an element before its first MediaElementAudioSourceNode is created. acquireMediaElementSource() returns the cached node before reclassifying the element. If a reused <audio> node first plays a same-origin source and later its src or <source> changes to cross-origin without CORS, direct callers reconnect that cached node and get the exact spec-mandated silence this PR fixes. The runtime caller classifies first and skips the transport, but the element was already permanently rerouted by the cached node; if decode fails, its claimed native fallback is still silent. Dynamic source updates on an existing DOM node therefore remain broken.

Please cover the same-element same-origin-to-cross-origin transition and make the route safe after a node has already been created (or explicitly replace or recreate the media element before native fallback). The current tests only exercise fresh elements, so they cannot see the one-way cached-node case. No merge action.

— Magi

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concur with @magi-bot CHANGES_REQUESTED at cce17da5. The cached MediaElementAudioSourceNode reuse is the primary blocker. Adding orthogonal concerns.

Concur with @magi-bot (verified at head):

  • WebAudioTransport.acquireMediaElementSource (packages/core/src/runtime/webAudioTransport.ts:263-273) returns _mediaElementSources.get(el) before the classifier runs. Grepped: _mediaElementSources is only invalidated in destroy() (line 753) — no emptied/abort/loadedmetadata-driven cache clear. Same-origin → cross-origin src mutation permanently holds the cached node; the element's native output stays hijacked per WebAudio spec; the classifier's "native fallback" is spec-impossible for that element.
  • decodeAudioElement becomes the ONLY working audio; if decode fails the element is silent — the same failure #3458 is meant to fix, now blessed by the runtime.
  • No test exercises reused elements (init.test.ts / webAudioRoute.test.ts construct fresh per case).

Additional (differentiated):

  1. Bind-time false-positive diagnostic on <source> fallback selection. init.ts:1871-1890 calls reportWebAudioRoute(mediaEl) synchronously at bind. webAudioRoute.ts:801-813 routeCandidates(): when both currentSrc and src attr are empty, walks <source> children and returns "decode-only" on the FIRST cross-origin URL. Scenario: <audio><source src="https://cdn.example.com/a.mp3"><source src="/assets/fallback.mp3"></audio>. Bind fires with no committed resource → conservative decode-only verdict → reportWebAudioMediaRoute emits [hyperframes] runtime_web_audio_bypass + latches diagnosedElements.add(el). Later loadedmetadata fires with currentSrc = /assets/fallback.mp3 (same-origin, browser-selected). Classifier now returns web-audio, no report. But the false-positive diagnostic already fired and the CLI check gate reports a phantom bypass. Fix: emit only from the loadedmetadata handler, or only latch after currentSrc is set.

  2. hasCorsOptIn secondary IDL check is over-permissive. webAudioRoute.ts:78-82:

    if (hasAttr(el, "crossorigin")) return true;
    return typeof el.crossOrigin === "string";

    In Chromium/Firefox/Safari, el.crossOrigin for an element with no attribute returns null (typeof "object"), so the fallback is inert. But in jsdom variants (and any host that returns "" for absent-attribute IDL), the fallback returns true for ALL cross-origin audio, silently disabling the entire guard in test envs. A genuine IDL-set el.crossOrigin = "anonymous" reflects to the attribute → primary check catches it. Recommend return typeof el.crossOrigin === "string" && el.crossOrigin.length > 0 — the empty-string fallback buys nothing and risks fail-open.

  3. Comment overstates enforcement reach. webAudioTransport.ts docstring: "init.ts routes on the same verdict before ever calling in; this stays the enforcement point so a direct caller (studio, player) cannot reopen the one-way door." But packages/studio/src/components/sidebar/AudioRow.tsx:149 calls actxRef.current.createMediaElementSource(el) DIRECTLY, bypassing WebAudioTransport entirely. Same-origin serveUrl in dev makes this benign today, but the enforcement claim is factually wrong. Either correct the comment or route AudioRow through the transport.

  4. srcObject / MediaStream binding unclassified. routeCandidates only walks currentSrc/src/<source>. el.srcObject = mediaStream leaves all three empty → classifier returns web-audio → MediaElementSource proceeds. For MediaStream, spec-fine. For MediaSource attached via srcObject (streaming HLS/DASH from a cross-origin CDN), MediaElementSource may still be silenced. Low prevalence in current HF compositions; document in the module preamble.

  5. Redirect chains bypass classification. isCorsSilenced (webAudioRoute.ts:822-833) judges the raw URL string, not the resolved fetch. <audio src="/proxy/track.mp3"> where /proxy 302s to https://cdn.example.com/track.mp3 → same-origin verdict → capture proceeds → spec silences. Fundamental gap without a HEAD probe or fetch-hook; document.

Softer-floor suggestion on @magi-bot's remediation: since the DOM <audio> node is composition-owned (not runtime-owned), a hard "explicitly recreate the element" is expensive. Softer: (a) listen for emptied/loadstart on bound media, _mediaElementSources.delete(el) + disconnect the stale node before new src loads; (b) if verdict flips to decode-only on a cached element, log a distinct DIAGNOSTIC_CACHE_POISONED code — since native recovery is spec-impossible, at minimum the failure has to be OBSERVABLE (matches the whole point of this PR).

— Review by tai (pr-review)

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES at acc6898

R2 delta verification (base cce17da5 → head acc6898).

Addressed cleanly (4/4 R2 claims verified):

  1. Bind-time false-positive (tai#1). isRouteSelectionSettled() gate added in webAudioRoute.ts; init.ts:1847-1852 skips the discovery-time report when both currentSrc is empty and src attr is absent. The loadedmetadata listener still fires with a settled currentSrc and calls reportWebAudioMediaRoute (latched per-element, so single report). No race: gate + classify are both synchronous with no microtask boundary between them, and routeCandidates reads the same currentSrc/src the gate just read.
  2. hasCorsOptIn (tai#2). Reduced to el.crossOrigin != null. Two new regression tests via withUnreflectedCrossOrigin (Object.defineProperty) cover both directions. Miga's != null correctly respects <audio crossorigin> — the bare-attribute anonymous opt-in whose IDL fallback is "". tai's suggested .length > 0 would have wrongly rejected that valid opt-in; Miga's trade-off is more spec-faithful. Untouched element (crossOrigin === null) still classifies as no-opt-in.
  3. AudioRow bypass (tai#3). AudioRow.tsx:172 now sets el.src = serveUrl BEFORE the classifier read (necessary — classifier reads currentSrc/src), then gates createMediaElementSource(el) behind classifyWebAudioMediaRoute(el).kind === "web-audio". Falls through to native <audio> playback (no visualizer) when the verdict blocks capture. Grep confirms AudioRow was the only other direct createMediaElementSource call site outside the transport (the hit in packages/lint/src/rules/media.ts is a rule reference, not a call). Public subpath @hyperframes/core/runtime/web-audio-route wired via package-subpaths.json + package.json exports.
  4. srcObject (tai#4). Documented in webAudioRoute.ts module docstring as "recorded as a boundary rather than fixed" — defensible: no current codepath feeds createMediaElementSource from a srcObject element.

BLOCKER unresolved — Magi's R1 primary finding:

WebAudioTransport.acquireMediaElementSource (webAudioTransport.ts:262-274) is untouched in this delta:

private acquireMediaElementSource(el) {
  const cached = this._mediaElementSources.get(el);
  if (cached) return cached;   // ← returns before reclassifying
  ...
  const route = classifyWebAudioMediaRoute(el);
  if (route.kind !== "web-audio") { reportWebAudioMediaRoute(el, route); return null; }
  const sourceNode = this._ctx.createMediaElementSource(el);
  this._mediaElementSources.set(el, sourceNode);
  return sourceNode;
}

_mediaElementSources is still only cleared in destroy() (line 753) — no emptied / loadstart / abort-driven eviction. The same-element same-origin-then-cross-origin src mutation Magi described therefore remains broken: the cached MediaElementAudioSourceNode created against the original same-origin src is returned on the reused-element path, permanently rerouting the element's native output per the Web Audio spec even though a fresh classify would now say decode-only.

The AudioRow classifier gate does not cover this case — that fix protects the studio preview player, not runtime element reuse in the transport. And no transport-level test exercises reused elements: webAudioTransport.test.ts:167 covers a fresh cross-origin element only, and line 150's "cached native source reusable" test is same-origin throughout.

Also unresolved (soft):

  • tai#5 (redirect chains). isCorsSilenced still judges the raw URL string; not documented in the delta.
  • webAudioTransport.ts docstring on acquireMediaElementSource still claims to be "the enforcement point so a direct caller (studio, player) cannot reopen the one-way door." AudioRow (a studio direct caller) demonstrates it is one of two co-equal enforcement points; the softened claim in webAudioRoute.ts acknowledges this, but the transport's own docstring does not. Minor.

Minimum ask:

Either (a) invalidate _mediaElementSources on emptied / loadstart and disconnect the stale node before the next classify, or (b) if out of scope, document the cached-node hazard in the module docstring alongside the srcObject boundary and add a distinct DIAGNOSTIC_CACHE_POISONED observable code so failures are at least surfaced (per tai's soft-floor suggestion). Either resolves Magi's block; leaving it silent-and-undocumented does not.

CI at time of review: Producer unit tests SUCCESS, Lint SUCCESS, Fallow audit SUCCESS, Typecheck in progress; regression + windows-render + player-perf shards still running. No merge action.

— Via

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 at head acc68982 after Miga's fix-up. Items 1, 3, 4 + @magi-bot's cached-node reuse are addressed well; Item 5 (redirect chains) is explicitly deferred (acceptable). One residual defense-in-depth gap on the transport worth flagging, but no code-level blocker at head.

Retraction on my R1 Item 2 — I was wrong.

My R1 recommendation to tighten hasCorsOptIn to typeof el.crossOrigin === "string" && el.crossOrigin.length > 0 is spec-wrong. Per HTML spec crossorigin is an enumerated attribute whose invalid-value default is anonymous; therefore crossorigin="" IS a valid opt-in equivalent to anonymous, and its IDL fallback reads as "". Miga's != null at webAudioRoute.ts:80-83 is the correct predicate. My length > 0 suggestion would have falsely rejected legitimate opt-ins for hosts that expose the value only via the IDL property (not reflected to the attribute), reintroducing the silent-audio bug this PR is meant to fix. The module comment at webAudioRoute.ts:61-79 calls this out explicitly, and the tests at webAudioRoute.test.ts:60-68, 114-138 assert both the reflected-attribute path and the unreflected empty-IDL path correctly. Withdrawing the R1 recommendation.

Concur on the fixes (verified at head):

  • Item 1 (bind-time false-positive)isRouteSelectionSettled(el) at webAudioRoute.ts:150-154 returns true only when currentSrc or src attr is set. init.ts:1845-1855's discovery-time reportWebAudioRoute skips when unsettled; init.ts:1898 binds loadedmetadata for the deferred report. Latch in reportWebAudioMediaRoute (webAudioRoute.ts:238-240) only fires on non-web-audio verdicts, so the early-skip does not consume the latch. Edge case: if <source> selection genuinely never settles (all candidates fail to load), discovery-time won't fire, BUT the schedule path at init.ts:3170-3171 still calls reportWebAudioMediaRoute(rawEl, route) when the classifier's <source>-walk produces a decode-only verdict — so an authored composition that ever tries to schedule the audio still emits. Acceptable trade-off.
  • Item 3 (AudioRow bypass)AudioRow.tsx:174 sets el.src = serveUrl before classifying; :184 calls classifyWebAudioMediaRoute(el) and only wires createMediaElementSource on .kind === "web-audio". Non-web-audio verdict is a graceful no-op: analyser/visualizer bar skipped, but native <audio> playback below (:194 audioRef.current.play()) still runs. New public subpath @hyperframes/core/runtime/web-audio-route correctly declared in packages/core/package-subpaths.json (import, browser export, CommonJS export all present).
  • Item 4 (srcObject / MediaStream) — module docstring at webAudioRoute.ts:30-36 is explicit and useful: "a srcObject element always reads as web-audio here, correctly or not. Nothing in this codebase feeds createMediaElementSource from a srcObject element today, so this is recorded as a boundary rather than fixed." Documented deferral is fine.
  • @magi-bot's cached-node reuse — closed at the caller side. init.ts:3170 calls classifyWebAudioMediaRoute(rawEl) fresh on every schedule invocation and only invokes scheduleMediaElementPlayback on .kind === "web-audio", so a src flip from same-origin (cached node exists) to cross-origin can no longer trigger the poisoned-cache path via the timeline scheduler.

One residual defense-in-depth gap (non-blocking, scope call):

WebAudioTransport.acquireMediaElementSource at webAudioTransport.ts:262-274 still returns this._mediaElementSources.get(el) unconditionally when cached, WITHOUT re-classifying — the classifyWebAudioMediaRoute call at :266 only runs on cache miss. So the transport's docstring at :258-260 ("this stays the enforcement point so a direct caller … cannot reopen the one-way door") overclaims: the enforcement only holds on FIRST bind. init.ts:3170's pre-classify closes the observed path today, but any future direct caller of acquireMediaElementSource (or a refactor of init.ts that stops pre-classifying) touching an element whose src was flipped after first cache would hand back a stale silent node.

Two ways to close it:

  • (a) Invalidate the _mediaElementSources entry when src changes — element listener for emptied/loadstart or comparing currentSrc against the value cached at first bind.
  • (b) Move the classifyWebAudioMediaRoute check ahead of the cache return inside acquireMediaElementSource — reject cached nodes for elements whose current verdict is no longer web-audio.

Scope call — happy to see either in this PR or a follow-up.

Minor:

  • Redirect-chain boundary (Item 5) is deferred but not documented in isCorsSilenced — a parallel doc line matching the srcObject boundary note (webAudioRoute.ts:30-36) would be honest.
  • Transport docstring at :258-260 still says "enforcement point"; Miga's Slack described "softened to caller contract," but that exact phrase isn't in the code. Downstream of the defense-in-depth call above — if you close the cached-node re-classify, the "enforcement point" claim becomes literally true and the comment can stay.

CI status at head: MANY required checks still IN_PROGRESS (Build, CLI smoke, Typecheck aggregate, Tests, Tests on windows-latest, all 9 regression-shards, Render on windows, Preview parity, Producer integration, Perf suite). Not stamping until CI settles.

Test coverage on the new work is strong — webAudioRoute.test.ts has 12 tests including both crossorigin-attr-reflection paths and the <source> walk. Good.

— Review by tai (pr-review)

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 32ef37c. R3 delta from my R2 (COMMENTED, pullrequestreview-5015279759): CI has settled all-green at head, and one CI-only tsconfig line was added. Nothing substantive changed since R2's verifications, so moving to APPROVE.

acc6898232ef37c diff (whole change):

packages/core/tsconfig.json:23 adds "src/runtime/webAudioRoute.ts" to the composite-build files allowlist so the new @hyperframes/core/runtime/web-audio-route subpath export resolves under tsc --build. git diff acc68982 32ef37c touches only that one line. Source unchanged, no consumer moves, no runtime behavior change. Pure typecheck hygiene.

Fixes verified at 32ef37c (unchanged bytes since R2 concur):

  • R1 item 1 (isRouteSelectionSettled gate)packages/core/src/runtime/webAudioRoute.ts:150-154 predicate (settled iff currentSrc OR src attr set) + init.ts:1853 early-skip in reportWebAudioRoute + init.ts:1859 deferred loadedmetadata refire. Latch at webAudioRoute.ts:238-240 only fires on non-web-audio verdicts, so the early-skip doesn't consume it. Tests at webAudioRoute.test.ts:141-159 cover both directions.
  • R1 item 2 (hasCorsOptIn)webAudioRoute.ts:80-82if (hasAttr(el, "crossorigin")) return true; return el.crossOrigin != null; correctly encodes the enumerated-attribute rule. crossorigin="" IS a valid opt-in whose IDL fallback is "anonymous"; my R1 .length > 0 recommendation was spec-wrong and correctly rejected. Docstring at :61-79 and regression tests at webAudioRoute.test.ts:114-138 (unreflected-empty-IDL path via withUnreflectedCrossOrigin) both cover it.
  • R1 item 3 (AudioRow bypass)packages/studio/src/components/sidebar/AudioRow.tsx:172 sets el.src before classify; :184 gates createMediaElementSource behind classifyWebAudioMediaRoute(el).kind === "web-audio". git grep -n createMediaElementSource confirms AudioRow + webAudioTransport are the only production call sites. Non-web-audio verdict skips the visualizer only; native <audio> playback still runs.
  • R1 item 4 (srcObject boundary)webAudioRoute.ts:30-36 module docstring documents the gap explicitly and notes nothing in the codebase feeds createMediaElementSource from a srcObject element today.
  • @magi's cached-node — closed at the caller side: init.ts:3170 pre-classifies fresh on every schedule invocation.

CI at head: all required SUCCESS — Build, Lint, Typecheck, Fallow audit, all 9 regression-shards, Windows Tests + Render, Player perf (load/fps/scrub/drift/parity), Preview parity, Producer unit + integration, CLI smoke on ubuntu/macos/windows, Studio smoke, SDK unit/contract/smoke, Test: runtime contract, Test. CodeQL green.

Residual defense-in-depth (unchanged from R2, still my non-blocker):

WebAudioTransport.acquireMediaElementSource at webAudioTransport.ts:262-274 still returns the cached node before re-classifying; docstring at :258-260 still overclaims "enforcement point... direct caller cannot reopen the one-way door." Observable path is closed today by init.ts:3170's pre-classify — a future direct caller (or a refactor that drops the pre-classify) would reopen it. R2's three close options stand: (a) emptied/loadstart/abort cache invalidator, (b) move classify above the cache return, or (c) soften the docstring to reflect the "caller contract" claim. Follow-up is fine by me.

Merge gate: still BLOCKED on @vanceingalls's R2 CHANGES_REQUESTED at acc68982 and @magi-bot's R1 CHANGES_REQUESTED at cce17da5 — both explicitly blocked on the transport cached-node reuse. Their calls to lift; my APPROVE alone doesn't clear either. If the cached-node scope call stays "follow-up," respond on their reviews directly.

— Review by tai (pr-review)

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R3 delta @ 32ef37c — tai's feedback follow-up

acc68982 → 32ef37c diff is one line: packages/core/tsconfig.json:23 adds src/runtime/webAudioRoute.ts to the composite-build files allowlist, alphabetically between stackingContext.ts and wiggleEase.ts. gh api compare .files | length == 1. No other lane files added/removed, no scope regression; fixes tsc --build resolution for the @hyperframes/core/runtime/web-audio-route subpath declared in package-subpaths.json. All required CI green at head.

Verified (R2-fix carryover, bytes unchanged at head — spot-checked at 32ef37c):

  • isRouteSelectionSettled() gatewebAudioRoute.ts:150-154 predicate = currentSrc || hasAttr(el,"src"). Pure DOM read, no boolean state to reset. init.ts:1853 early-skips the discovery-time reportWebAudioRoute; loadedmetadata handler bound at :1898 refires post-selection. Gate + classifyWebAudioMediaRoute are both synchronous inside reportWebAudioRoute, no microtask boundary — no race window. Latch at webAudioRoute.ts:238-240 trips only on non-web-audio verdicts, so the early-skip doesn't consume it.
  • Empty crossOriginwebAudioRoute.ts:80-82 = hasAttr(el,"crossorigin") || el.crossOrigin != null. null (real-browser absent-attribute IDL) rejected via != null; undefined covered too. crossorigin="" (enumerated-attribute anonymous opt-in, IDL fallback "") accepted via the primary attribute check — my R1/R2 .length > 0 push was spec-wrong and correctly rejected. Explicit default: any hasAttr short-circuits true. Docstring at :61-79 walks the spec.
  • AudioRow bypassAudioRow.tsx:172 sets el.src = serveUrl before classify, :184 gates createMediaElementSource behind .kind === "web-audio". git grep -n createMediaElementSource packages/ at head — AudioRow + WebAudioTransport.acquireMediaElementSource are the only production call sites (packages/lint/src/rules/media.ts is a rule reference). Non-web-audio verdict is a graceful skip: visualizer dark, native <audio> playback at :194 still runs.
  • srcObject gapwebAudioRoute.ts:30-36 module docstring. First-principles explanation: routeCandidates only reads src-shaped attributes, so a srcObject element always reads as web-audio here. Present-scope closure: nothing in the codebase feeds createMediaElementSource from a srcObject element today. Not a workaround-cite.

Non-blocking / follow-up:

  • My R2 CHANGES_REQUESTED at acc68982 still stands — the R3 delta doesn't touch it. WebAudioTransport.acquireMediaElementSource at webAudioTransport.ts:262-274 is byte-identical at 32ef37c: cached node returned before re-classify, _mediaElementSources still only cleared in destroy(). Observable path is closed today by init.ts:3170's pre-classify (confirmed at head), but the transport docstring at :258-261 still overclaims "enforcement point... direct caller cannot reopen the one-way door." Concurring with @terencecho's R3 close options — cache-invalidator on emptied/loadstart, classify above cache return, or soften the docstring to reflect the caller-contract shape. Not raising as a fresh R3 blocker (the delta doesn't touch it), not lifting the R2 CR either.
  • Redirect-chain gap in isCorsSilenced (R1 item 5) still undocumented; a parallel doc line to the srcObject boundary note (webAudioRoute.ts:30-36) would be honest.

— Via

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR state: CHANGES_REQUESTED (Via R2 CR at acc68982 still standing; magi-bot R1 CR at cce17da5 still standing). All required CI green at head. R3 delta from acc6898232ef37c is one line (packages/core/tsconfig.json:23 adding webAudioRoute.ts to the composite-build files allowlist) — source and runtime behavior unchanged since R2. Reviewer positions at HEAD have converged more than the framing suggested: Via's R3 COMMENTED at 32ef37c explicitly says "Observable path is closed today by init.ts:3170's pre-classify (confirmed at head)" — the CR persists at the R2 SHA because the R3 delta doesn't touch what R2 flagged, not because Via still believes the observable audio bug reproduces at HEAD.

Blockers
• None net-new from independent read. My arbitration verdict below aligns with tai's observable path closed at head — and with Via's own R3 concession. Whether the residual is a blocker is a scope call, not a correctness call.

Concerns
packages/core/src/runtime/webAudioTransport.ts:262-274acquireMediaElementSource returns the cached MediaElementAudioSourceNode before re-classifying, and _mediaElementSources is cleared only in destroy() (line 753). This is bytes-identical to base. Observable audio-silence path IS closed at head because init.ts:3170 pre-classifies BEFORE the transport is asked and short-circuits the transport call (capture = ... ? scheduleMediaElementPlayback(...) : Promise.resolve(null)) — so acquireMediaElementSource is never reached with a cross-origin src. But the RESIDUAL (element internally rerouted from the earlier same-origin capture) survives a src mutation to cross-origin: on the fallback decode path, if that cross-origin CDN also refuses CORS, decodeAudioElement returns null, the runtime falls through without muting (route ≠ web-audio, so the non-unit-rate mute gate is skipped by design at init.ts:3204-3211), and the element attempts native output while the browser is still holding the MediaElementSource routing — silence. This is a NARROW subset of #3458 (was: any cross-origin element; now: only reused, previously-captured, mutated to cross-origin, decode also CORS-blocked). Strictly better than pre-PR state, still latent.
webAudioTransport.ts:258-261 docstring: "this stays the enforcement point so a direct caller (studio, player) cannot reopen the one-way door" — as Via notes, AudioRow.tsx:184 demonstrates that direct callers actually enforce THEIR OWN classify gate. The docstring reads as if acquireMediaElementSource is a self-sufficient guard when it is really a caller-contract. Softening this is the cheapest close of Via's R2.
webAudioRoute.ts:123-134 isCorsSilenced judges the raw URL's origin, not the FINAL resolved URL after redirects. A same-origin-looking URL that 302s to a cross-origin CDN silently returns "web-audio" and reintroduces the bug at fetch time; conversely a cross-origin URL that 302s to same-origin gets its Web Audio graph withheld unnecessarily. This is Via's R1 item 5 and is still undocumented in the delta. Boundary line parallel to webAudioRoute.ts:30-36's srcObject note would honestly disclose the gap.
applyVariableBindings.ts:174 (el.setAttribute("src", url)) is the ONE production path I found that can flip a live <audio> element's src origin at runtime. It runs at init and after external/inline composition load (init.ts:2390) — not on every seek — so the reused-element residual is narrow but plausibly reachable in real compositions using data-var-src when a composition-load pass resolves a var to a different origin than the initial bind. Not currently covered by any test.

Nits
webAudioRoute.ts:83el.crossOrigin != null correctly covers the untouched-null AND unreflected-"" cases per tests at webAudioRoute.test.ts:114-138. Would read cleaner as an explicit typeof el.crossOrigin === "string" if the IDL semantics matter to a future maintainer — the != null idiom is spec-right but visually invites the wrong "why isn't this a truthiness check" question the docstring already had to defend. Docstring at :61-79 does the work; this is aesthetic.
init.ts:1854if (!(mediaEl instanceof HTMLAudioElement)) return; inside reportWebAudioRoute. The comment above correctly justifies audio-only, but the caller bindMediaMetadataListeners binds loadedmetadata on both audio + video. Consider hoisting the audio-guard to the binding side to avoid installing a listener whose handler will always no-op for <video>.

Questions
• Does hyperframes have any production paths beyond data-var-src that mutate a live <audio> element's src origin? Studio operations, template hot-swap, sub-composition rebind? If none, the residual moves from "reachable-but-narrow" to "boundary-only" and Via's R2 CR is fair to close as a docstring softening. If yes, the invalidator becomes the honest fix.
• Miga — is a follow-up ticket for the transport-level cache invalidator + redirect-chain doc-boundary acceptable to you and Via as the close path, or do you want them in this PR?

Arbitration verdict — cached-node dispute
🟡 Nuanced. tai and Via are actually agreeing more than the framing suggested; the daylight between them at HEAD is scope, not correctness. Concretely:

  1. Fresh cross-origin (<audio src="https://cdn/..." > never previously captured): closed by init.ts:3170-3186. Verified in test at init.test.ts:3120-3136. tai + Via + Miga concur.
  2. Reused same-origin→cross-origin, decode-friendly CDN: closed via decode buffer path. init.ts:3170 classifies decode-only → capture=Promise.resolve(null) → fallback fires → webAudio.decodeAudioElement(rawEl) fetches the mutated src → schedulePlayback builds an independent AudioBufferSourceNode and sets rawEl.muted = true (webAudioTransport.ts:594). Buffer plays through its own graph, independent of the cached MediaElementSource. Audible.
  3. Reused same-origin→cross-origin, CORS-blocked CDN: STILL BROKEN. init.ts:3170 classifies decode-only → capture is null → decodeAudioElement's fetch fails (opaque response, swallow returns null) → schedulePlayback never called → element is NOT muted by the runtime → element attempts native output → browser retains the MediaElementSource routing from the earlier same-origin capture → native output silenced by the internal reroute. Element plays silently.

The bytes that close cases (1) and (2): init.ts:3170-3171 (the pre-classify + report), init.ts:3175-3186 (the route.kind === "web-audio" ? ... : Promise.resolve(null) gate), and init.ts:3212-3226 (the decode fallback). tai's "pre-classify closes the observable path" is precisely right for cases (1) and (2).

The bytes that leave case (3) latent: webAudioTransport.ts:263-264 (const cached = this._mediaElementSources.get(el); if (cached) return cached;) — dead code for case (3) since acquireMediaElementSource isn't reached at HEAD via init.ts. But the RESIDUAL routing lives at the browser-DOM level, not in the WeakMap: once _ctx.createMediaElementSource(el) fires (webAudioTransport.ts:271), the element is permanently rerouted browser-side per Web Audio spec, and stopAll() only calls source.sourceNode.disconnect() (webAudioTransport.ts:681) — the node stays bound to the element. Via's R2 CR says the same-element same-origin→cross-origin case "remains broken"; at HEAD that's true ONLY for the decode-fails subcase. tai is right that the OBSERVABLE-in-CI path is closed; Via is right that a residual silent-audio path survives for a specific production shape (reused element + data-var-src origin flip + CORS-hostile CDN).

Pre-PR baseline: ALL cross-origin silenced. This PR: only the reused+decode-fails subcase silenced. Strict improvement; not a regression. Whether it's a merge blocker depends on how likely data-var-src-driven src mutation crosses an origin boundary in the field.

Recommendation: land the R2 delta AS-IS with (a) Via's webAudioTransport.ts:258-261 docstring softening (5-min patch) + (b) a follow-up ticket for either the emptied/loadstart cache invalidator OR the DIAGNOSTIC_CACHE_POISONED observable code, whichever fits Miga's cost model. The docstring fix satisfies Via's stated R2 close criterion without demanding the invalidator in-PR.

Adversarial ledger
• Redirect-chain gap: isCorsSilenced misjudges both directions of a redirect. Not a regression, was silent before, still silent now — but Miga's PR is the moment to disclose it in the module docstring.
applyVariableBindings.ts:174 is the concrete live path that reaches the residual. Not exercised in the delta's tests.
srcObject gap acknowledged in webAudioRoute.ts:30-36. Fine.
• Non-unit-rate fail-closed rule: at init.ts:3209, route.kind === "web-audio" && hasProcessing — the intended asymmetry (mute FX+non-unit only when transport TRIED and failed) is correct per test coverage at init.test.ts:3225-3269. Behavior preserved.
WebAudioTransport docstring at :258-261 claim about being the "enforcement point" is factually false — AudioRow.tsx:184 IS an equivalent gate. Docstring lags reality.

Tests
webAudioRoute.test.ts covers the classifier well (283 lines, all verdict shapes including empty-string IDL fail-open guard at :114-124 and untouched-IDL fail-closed at :126-138).
webAudioTransport.test.ts:167-181 covers fresh cross-origin refusal; :124-134 covers same-src cache reuse; :150-160 covers stopAll leaving cache intact.
init.test.ts:3057-3271 covers the init.ts pre-classify path, native fallback with FX, non-unit-rate fail-closed carryover.
Coverage gap: no test exercises the reused-element same-origin→cross-origin src mutation on the transport OR on the init.ts orchestration. Given Miga's PR docstring at webAudioTransport.ts:249-260 and Via's/Magi's R1+R2 focus, a test asserting one of {schedulePlayback plays decoded buffer through independent graph; classifier rejects at next call} would be worth cheap.

Stamp stance
LGTM from my side on the substantive close of #3458 for the common cases (fresh cross-origin + reused-with-friendly-CDN). Leaving as COMMENT — I do not stamp this repo autonomously, and the residual + docstring softening should land or be follow-up-ticketed before a stamp. If Miga wants the R2 CR lifted in this PR, softening webAudioTransport.ts:258-261 per Via's request is the cheapest path; if not, a follow-up ticket referencing this arbitration is honest.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R4 delta @ f7fc001 — R2/R3 blocker addressed

Verified (delta acc6898..f7fc001, scope: webAudioRoute.ts +15 lines, webAudioTransport.ts +38/-4, webAudioTransport.test.ts +52, tsconfig +1):

  • Cached-node reclassify + evict path (webAudioTransport.ts, acquireMediaElementSource): cache-hit branch now calls the same pure classifyWebAudioMediaRoute(el) used on cache-miss — symmetric enforcement, no drift risk. On a non-web-audio verdict the sequence is disconnect-inside-try/catch → _mediaElementSources.delete(el)reportWebAudioMediaRoutereturn null. Order is right (disconnect before eviction), the classifier is pure and swallows URL parse errors internally so it cannot throw, and the diagnostic key runtime_web_audio_bypass is a stable string constant. No cross-tick await between the cache read and eviction, so no concurrent-capture race window in single-threaded JS.

  • Docstring correction: prose changed from "this stays THE enforcement point so a direct caller (studio, player) cannot reopen the one-way door" to "this method is A enforcement point, not THE enforcement point; every caller that can reach createMediaElementSource is expected to classify first" — and names both peer enforcement sites (init.ts routing, AudioRow.tsx's preview player). Matches the actual topology.

  • Redirect-chain doc: 15-line JSDoc addition to WebAudioMediaRoute explains WHY from first principles — "following the chain to inspect the final response would turn a pure, synchronous verdict — needed on every schedule call — into an async fetch" — and notes both directions (same→CDN and CDN→same-origin). Reads as a documented boundary, not a shrug.

  • Regression tests (webAudioTransport.test.ts, "stops returning the cached node once the same element's src moves cross-origin" and its same-origin twin): use the REAL classifier (grep confirms no vi.mock of webAudioRoute), simulate a genuine src mutation via el.setAttribute("src", "https://cdn.example.com/reused-clip.mp3"), then assert observable eviction — createMediaElementSource call count stays at 1 (no rebuild attempt over the one-way door), mediaElementSourceNode.disconnect called exactly once after a mockClear that scrubs stopAll()'s own disconnect, and second === null. Same-origin twin asserts the inverse (returns non-null, disconnect NOT called). This is R2's exact scenario — pooled element, src moves same-origin → cross-origin between capture attempts — and it now hands back null for the decode-only fallback instead of the stale silent node.

Non-blocking / follow-up:

  • Tests assert eviction via the disconnect count + return-value pair rather than a direct _mediaElementSources.has(el) === false check. Observationally equivalent for the current cache-hit flow, but a direct Map assertion would fail more loudly on future refactors.
  • unbindMediaMetadataListeners in init.ts unbinds onMediaLoadedMetadataForRoute on runtime teardown; the transport's cache eviction is independent and doesn't touch those listeners — fine (they belong to a different concern), just worth remembering if a future change tries to consolidate lifecycle.

Signature: — Via

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE (re-approve) at f7fc0017a6a6f31a4ae725f9f391d8dc63e3d9cc — HF public dismisses APPROVE on push, so this reinstates the R3 stamp (pullrequestreview-5025440300) after the fix-up commit.

Delta since 32ef37c is one focused commit that closes @vanceingalls's R2 cached-node-reuse blocker (also the non-blocking residual I flagged at R2/R3):

  • webAudioTransport.ts:272-298acquireMediaElementSource now reclassifies on every cache hit; when the verdict is no longer web-audio it disconnects the stale node (try/catch swallowing "already torn down"), deletes from _mediaElementSources, calls reportWebAudioMediaRoute, and returns null. Sole caller (scheduleMediaElementPlayback:333) already handles nullinit.ts:3169's capture.then routes to the decode fallback. Ordering (disconnect → evict → report) matches the accepted pattern, and _mediaElementSources is the only element-keyed cache on the transport, so no companion map to co-evict.
  • webAudioTransport.ts:263-269 — docstring softened: this method is A enforcement point, not THE. AudioRow.tsx:184 verified to classify over its own throwaway AudioContext before createMediaElementSource:185, and init.ts:3169 pre-classifies before the transport call. Accurate.
  • webAudioRoute.ts:37-51 — redirect-chain gap documented alongside the srcObject boundary. Both directions noted (same-origin→cross-origin false-negative silence; cross-origin→same-origin false-positive fallback). Documented not fixed, same rationale as srcObject — no in-tree caller routes through redirects today.
  • webAudioTransport.test.ts:213-260 — two regressions: same-origin → cross-origin move evicts (second === null, createMediaElementSource count stays at 1, disconnect fired once) and same-origin → same-origin move keeps cache (second !== null, no re-build, no disconnect). Both isolate the fix's disconnect from stopAll's transient-graph disconnect via a mockClear(). Non-blocking nit: neither new test asserts the console.info diagnostic explicitly — the existing fresh-bypass test at :167-182 already pins that contract, so coverage stands.

Race analysis: reclassify runs synchronously after the sole await this._ctx.resume() in scheduleMediaElementPlayback:329. A src mutation landing in that await window is exactly the scenario the fix closes — no race introduced.

CI: Producer unit tests PASS (contains the transport suite); Producer integration + SDK unit/contract/smoke + Preview parity + Studio + Perf fps/load/drift/scrub + CLI smoke also green. Typecheck / Test / regression-shards / Windows / Perf parity still in-flight at review time; no failures.

Also closes the non-blocking residual I raised in R2/R3. @magi-bot's R1 CHANGES_REQUESTED at cce17da5 still stands on record — that only lifts when they re-review; my re-approve doesn't unblock merge on its own.

— Review by tai (pr-review)

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh R2 at exact head f7fc0017; my R1 blocker is closed.

webAudioTransport.ts:272-298 now reclassifies every cache hit with the same pure route classifier used for a miss. A same-origin → cross-origin src mutation disconnects and evicts the stale MediaElementAudioSourceNode, emits the bypass diagnostic, and returns null without attempting to reopen the one-way node-creation door. Same-origin reuse still returns the cached node.

webAudioTransport.test.ts:213-260 exercises both directions with the real classifier and proves no second createMediaElementSource call occurs. The transport docstring now describes its actual shared-enforcement topology, and the redirect boundary is documented. tai and Via independently covered the same exact-head delta; all hosted checks are green.

Verdict: APPROVE
Reasoning: The cached-node identity bug I blocked is fixed at the cache boundary itself, with mutation-specific regression coverage and no new node-creation path.

— Magi

@miguel-heygen
miguel-heygen merged commit 0c9d234 into main Aug 26, 2026
59 checks passed
@miguel-heygen
miguel-heygen deleted the fix/web-audio-cross-origin-silence-v2 branch August 26, 2026 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

createMediaElementSource silently mutes cross-origin media that has no CORS opt-in

5 participants